-
Notifications
You must be signed in to change notification settings - Fork 23
Transparent Proxy: Seamless handling of Cross-level consumption of destination-fragment pairs #903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
32591b7 to
c2a9936
Compare
c2a9936 to
05d9efa
Compare
MatKuhr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also update the PR description and sign the CLA 😉
.../src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxyDestination.java
Outdated
Show resolved
Hide resolved
| * @return This builder instance for method chaining. | ||
| */ | ||
| @Nonnull | ||
| public DynamicBuilder destinationLevel( @Nonnull final String destinationLevel ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is either "subaccount" or "instance". For such cases, we usually use enums and, in fact, just introduced one: https://github.com/SAP/cloud-sdk-java/blob/main/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceOptionsAugmenter.java#L139
But I also assume the two From the test code below it looks like provider is also supported, then let's reuse the enum.provider options don't apply to transparent Proxy? If it's just two options and not 4, then you could also consider to default to subaccount and just add one method without args to explicitly change it to .instanceLevel()
What is the behavior of TP if nothing is given? I assume subaccount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are four possible values for both destinationLevel and fragmentLevel - "provider_subaccount", "provider_instance", "subaccount" and "instance". The behaviour of the TP if nothing is given matches that of the Destination Service - the "default level" is "instnace ".
I have applied the DestinationServiceOptionsAugmenter.CrossLevelScope enum which fits perfectly for this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "default level" is "instnace ".
for the /v2 consume API yes, for the /v1 find destination it will fallback to subaccount. So I assume TP uses /v2 under the hood?
Because the Cloud SDK behaves according to /v1 by default, and only if cross-level is enabled, it uses /v2...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. The Transparent proxy works analogically - it uses the /v2 consume API when levels are defined in a either in a destination CR or as headers (the approach used in TP-CloudSDK integration). Otherwise the /v1 API is used by default.
Got it, I now marked this PR as draft and we can merge it once the release is available. @panayotmarinov kindly ping us when the release is ready and the PR has been finalized (merge conflicts, PR description, etc.). Thanks! |
05d9efa to
aa3d017
Compare
a7f84a5 to
89da889
Compare
89da889 to
0ea88e6
Compare
The PR ready for review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@panayotmarinov if not provided, what is the default for the two? Maybe you can clarify by adding that info to the Javadoc. Otherwise LGTM!
Context
https://jira.tools.sap/browse/UC-8026
This pull request introduces the integration of seamless handling of cross-level consumption of destination-fragment pairs with the Transparent Proxy. As a user, this allows you to consume these pairs on different levels, such as the subaccount and subscription level, enabling more flexible SaaS setup configurations without the need for duplication.
Feature scope:
Definition of Done
Error handling created / updated & covered by the tests aboveDocumentation updatedRelease notes updated